fix: align Node runtime guard with package engines - #305

No actionable comments were generated in the recent review. 🎉

Walkthrough

The Node.js runtime guard now enforces the package engine windows, including minor-version thresholds and excluded major versions. Startup and doctor diagnostics use the shared supported range. Tests validate boundaries and synchronization with package.json.

Changes

Node engine validation

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• TestSprite/testsprite-cli#11: Extended the runtime guard and its tests with supported engine ranges.
• TestSprite/testsprite-cli#183: Updated the Node.js version guard used by doctor.ts.
• TestSprite/testsprite-cli#230: Modified Node.js version support messaging and validation.

Suggested reviewers: ruili-testsprite

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

@ruili-testsprite @zeshi-du — this is ready for maintainer review: current head d1e697c, mergeable, focused on #254, with 14/14 boundary regressions passing and CodeRabbit reporting no actionable findings (pre-merge 5/5).

Could you please review the PR and confirm whether a merged solution for the assigned CLI task qualifies for the contribution bonus? I’ll address any requested change immediately and can provide the payout/onboarding details through your preferred private route after acceptance.

CI has now run (I approved the pending fork workflows) and it settles the "environment-sensitive" question:

• CI / Unit Tests (Node 20) — ubuntu-latest — ❌ src/commands/doctor.test.ts (19 tests | 11 failed)
• CI / Unit Tests (Windows) — ❌
• Test Coverage — ubuntu-latest — ❌ same 11
• Lint & Format, Typecheck, Build (20 + 22), Local E2E, gitleaks — all ✅

Same 11 failures, same file, on ubuntu/Node 20 as on my macOS/Node 26 control run — so they track the diff, not the machine. Everything else in the pipeline is green, which is a good sign for the rest of the change: fix the doctor fixture and the doctor message and this should go green in one push.

Run: https://github.com/TestSprite/testsprite-cli/actions/runs/31323659046

CI is now approved and has run — that's why you're suddenly seeing real failures after nine commits of guessing. Sorry: the fork-workflow approval was never granted, so you were iterating completely blind. That's the worst possible position to put a contributor in and it's on us.

Current state: Unit Tests fail on Node 20, Node 22 and Windows, and Coverage (>= 80%) fails as a consequence. Lint & Format, Typecheck and both builds pass.

The failures are concentrated in the doctor test fixtures — the guard change alters which Node versions are considered supported, and the fixtures that assert on doctor's output still encode the old expectations. Run npm test -- doctor locally and you'll see them all at once now; that's the whole set.

The underlying report (#254) is correct and accepted: src/version-guard.ts only checks major < 20 while package.json engines declare ^20.19.0 || ^22.13.0 || >=24, so 21.x, 23.x, and the excluded 20.0–20.18 / 22.0–22.12 patch ranges are all silently admitted. Two things worth getting right in the fix:

• the range check should be derived from or pinned against package.json engines, with a test that fails if the two drift apart — otherwise this bug comes back the next time engines change;
• no new runtime dependency for semver parsing. The dependency budget here is commander, valibot, undici only, and these ranges are simple enough to handle in-tree.

Ping me when it's green and I'll review promptly this time.

I approved the fork workflows again — they had gone back to action_required the moment you pushed ce5441b, so for the last 13 hours you were blind again through no fault of yours. Apologies; that gate re-arms on every push from a first-time contributor and I should be watching for it, not you.

CI and Test Coverage are green on ce5441b. Both blocking items from my review are resolved:

1. doctor.test.ts fixtures moved to 22.13.0 (L53 and L302), and the old an outdated Node runtime case was repurposed into an excluded in-range Node runtime fails the Node.js check at 22.9.0 — that's exactly the regression I asked for, and it's a better test than the one it replaced.
2. checkNodeVersion now reports SUPPORTED_NODE_RANGE on both the pass and fail paths, src/index.ts matches, and the stale .npmrc engine-strict comment is gone.

You also fixed the major-only regression I filed as non-blocking (minor = minorRaw === undefined ? 0 : Number(minorRaw)), and SUPPORTED_NODE_ENGINE is now pinned to package.json#engines.node by a unit test. Nice.

The red Security check is ours, not yours

Please don't chase it. Both failures are infrastructural:

• Dependency Review — fails on every PR in this repo because the dependency graph isn't enabled at the org level. Tracked separately.

• ESLint Security (changed files) — the job linted 27 files, but this PR only touches 5. It resolves its file list as git diff BASE_SHA github.sha, and on a pull_request event github.sha is the merge commit, not your head — so the diff also contains everything main gained while your PR sat waiting for review. main picked up five merged PRs yesterday, and their files (src/lib/secret-file.ts, src/lib/http.test.ts, src/lib/dry-run/samples.ts, src/commands/test.ts, src/commands/project.ts …) are what produced all 145 errors.

  Concretely: of the five files this PR actually changes, four report nothing, and src/index.ts reports one warning, which does not fail the job. Zero errors are attributable to this PR. The longer a PR waits here, the redder that check gets — which is a bug in our gate, and one that punishes exactly the contributors we kept waiting.

  This is already fixed in the private source repo (the gate now filters findings down to changed lines). The fix reaches this mirror on the next release, not before, so expect this check to stay red on your PR until then. I'll merge over it.

One thing left

Add a CHANGELOG.md entry under ## [Unreleased]. This is a user-visible break — anyone on 20.0–20.18, 21.x, 22.0–22.12 or 23.x runs the CLI fine today and gets a hard exit afterwards. That's the intended outcome of #254, but it needs to be written down where users will see it. One bullet is enough.

Push that and I'll approve and merge. My earlier Changes requested stays only until it lands.

The remaining item from my review (the range being encoded three ways — semver string, prose string, and the branch ladder, with only the semver string pinned by a test) I'm explicitly not blocking on. It's a fair follow-up if you want it, and equally fine to leave.